-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance CURLOPT_FTP_SKIP_PASV_IP #9758
Conversation
@bagder maybe ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a test case for this as well, verifying that it works
@@ -1198,7 +1198,7 @@ ParameterError getparameter(const char *flag, /* f or -long-flag */ | |||
config->ignorecl = toggle; | |||
break; | |||
case 'q': /* --ftp-skip-pasv-ip */ | |||
config->ftp_skip_ip = toggle; | |||
config->ftp_pasvp_ip_rule = CURL_FTP_SKIP_PASV_IP_ALWAYS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this breaks behavior for users who do --no-ftp-skip-pasv-ip
to switch off the option
@@ -177,7 +177,8 @@ struct OperationConfig { | |||
bool doh_verifystatus; | |||
bool create_dirs; | |||
bool ftp_create_dirs; | |||
bool ftp_skip_ip; | |||
long ftp_pasvp_ip_rule; /* how to handle the IP address the FTP server | |||
passes on to us */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should keep treating this as a boolean cmdline option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
original author permits 3 values, so I don't think bool would fit here.
Always ignore the IP returned by the server. | ||
.IP CURL_FTP_SKIP_PASV_IP_IF_NOT_ROUTABLE | ||
Only ignore the returned IP if it is within the private address space (see RFC1918), | ||
which is a common server configuration error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should also mention from which version this option works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, but I guess @Zenju has better knowledge of what he wanted to do with this
PR
@Zenju now that I squashed and refreshed your commits, would you please act on the comments? |
URL_FTP_SKIP_PASV_IP_ALWAYS instructs libcurl to ignore the IP address the server suggests in its 227-response to libcurl's PASV command when libcurl connects the data connection. Instead libcurl will re-use the same IP address it already uses for the control connection. But it will use the port number from the 227-response. The allowed values are: - CURL_FTP_SKIP_PASV_IP_NEVER Always use the IP returned by the server. - CURL_FTP_SKIP_PASV_IP_ALWAYS Always ignore the IP returned by the server. - CURL_FTP_SKIP_PASV_IP_IF_NOT_ROUTABLE Only ignore the returned IP if it is within the private address space (see RFC1918), which is a common server configuration error. Closes: #1455
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this breaks existing behavior
This can't be merged and no response in weeks. I would also like a stronger motivation why we want/need this feature. Letting the server decide IP address is a security risk. |
This is a refresh of #1470
I took the original authors PR, squashed it, and tried to refresh it towards the latest version of curl
URL_FTP_SKIP_PASV_IP_ALWAYS instructs libcurl to ignore the IP address the server suggests in its 227-response to libcurl's PASV command when libcurl connects the data connection. Instead libcurl will re-use the same IP address it already uses for the control connection. But it will use the port number from the 227-response.
The allowed values are:
CURL_FTP_SKIP_PASV_IP_NEVER Always use the IP returned by the server.
CURL_FTP_SKIP_PASV_IP_ALWAYS Always ignore the IP returned by the server.
CURL_FTP_SKIP_PASV_IP_IF_NOT_ROUTABLE Only ignore the returned IP if it is within the private address space (see RFC1918), which is a common server configuration error.
Closes: #1455